Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: mongodb integration test #154

Open
wants to merge 11 commits into
base: main
Choose a base branch
from

Conversation

cygaar
Copy link

@cygaar cygaar commented Dec 17, 2024

Previously, the mongo integration test was failing with Error connecting to Search Index Management service.. This is because it takes a bit of time for the container to initialize the service.

The extra delay added in #153 doesn't quite fix the issue because the index creation happens before the delay.

Our integration tests will now retry a couple of times before attempting to create the index.

@cvauclair
Copy link
Contributor

cvauclair commented Dec 17, 2024

Hey @cygaar thanks for the help in fixing this very annoying integration test! Retrying until the search index has been created is indeed the right way of fixing this. However, create_search_index doesn't actually return an error if the search index is not ready (so your loop will always exit on the first iteration).

Instead, I think the way to do this is to poll the list_search_indices function until the search index has status == READY. Something like:

// in `create_search_index` after creating the index
while attempts < max_attempts {
    match collection
        .list_search_indexes()
        .name(index_name)
        .await?
        .with_type::<rig_mongodb::SearchIndex>()
        .next()
     {
         Some(index) if index.status == "READY" => return,
         Some(_) => continue
         None => panic!() 
     }
}

@cygaar
Copy link
Author

cygaar commented Dec 17, 2024

I'll fix shortly!

@cygaar
Copy link
Author

cygaar commented Dec 17, 2024

@cvauclair I'm pretty sure the logic I have here works - I added a log to check if the index was created and it seems to work.
I checked the definition of create_search_index and it does return a value.

Screenshot 2024-12-17 at 12 27 40 PM

@cvauclair
Copy link
Contributor

cvauclair commented Dec 17, 2024

@cygaar apologies I wasn't very clear. The value returned by Collection::create_search_index is a builder struct, but the request is only executed when you call .await on the builder, and the return value is a Result<String, mongodb::Error> which will only be Err if the command was not successful, but Ok does not mean that the index is necessarily ready.

The docs suggest using list_search_indexes to check the status of an index (see the "important" section here).

@cygaar
Copy link
Author

cygaar commented Dec 17, 2024

ah cool, i'll look into that

@cvauclair
Copy link
Contributor

cvauclair commented Dec 17, 2024

FYI I had increased the delay further to 30s in another PR that has since been merged (hence the merge conflict), but feel free to reduce it back to 5s (or remove it entirely) once the polling for READY status is done!

@cygaar
Copy link
Author

cygaar commented Dec 17, 2024

I will note that the creation of the search index is what's causing the original error - we would essentially need to keep the logic i currently have and add an additional check to make sure the index was created

@cygaar
Copy link
Author

cygaar commented Dec 18, 2024

@cvauclair i think the last test failure is related to missing api keys - not entirely sure how you guys have github actions setup to get that to work

rig-mongodb/tests/integration_tests.rs Outdated Show resolved Hide resolved
rig-mongodb/tests/integration_tests.rs Outdated Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants